Enrich audit log entries with changed field details#584
Conversation
…ions Board, Card, Column, and Label services now capture pre-mutation state and build human-readable change summaries listing which specific fields changed. Null/unchanged fields are omitted. Tests verify change details appear in audit entries for all four services. Closes #583
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Adversarial self-reviewReviewed: Full diff ( FindingsNo bugs or security issues found. Specific checks performed:
Verdict: Ship-ready. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the audit logging system across the Board, Card, Column, and Label services by capturing pre-mutation states and generating detailed change summaries for update operations. The review feedback identifies several opportunities to reduce log noise by ensuring only actual value changes are recorded. Additionally, it highlights a logic error in the card blocking summary and suggests using more specific audit actions for unarchiving boards to improve consistency.
| if (dto.IsBlocked.HasValue && dto.IsBlocked.Value != oldIsBlocked) | ||
| parts.Add(dto.IsBlocked.Value ? $"Blocked: {dto.BlockReason ?? "no reason"}" : "Unblocked"); |
There was a problem hiding this comment.
The logic for logging a card as "Blocked" is incorrect. It logs the change if dto.IsBlocked.Value is true and different from oldIsBlocked, but it doesn't consider that the card is only actually blocked if !string.IsNullOrEmpty(dto.BlockReason). This can lead to logging a "Blocked" event when the card's state doesn't change, for instance if a request is made with isBlocked: true but no blockReason. The summary-building logic should mirror the update logic in UpdateCardAsync.
if (dto.IsBlocked.HasValue)
{
// A card is only blocked if a reason is given.
if (dto.IsBlocked.Value && !string.IsNullOrEmpty(dto.BlockReason) && !oldIsBlocked)
{
parts.Add($"Blocked: {dto.BlockReason}");
}
// A card is unblocked if the flag is set to false and it was previously blocked.
else if (!dto.IsBlocked.Value && oldIsBlocked)
{
parts.Add("Unblocked");
}
}| else if (dto.IsArchived == false) | ||
| await SafeLogAsync("board", board.Id, AuditAction.Updated, changes: $"unarchived; name={board.Name}"); | ||
| await SafeLogAsync("board", board.Id, AuditAction.Updated, changes: changeSummary); |
There was a problem hiding this comment.
When un-archiving a board (dto.IsArchived == false), the audit log action is Updated. For better clarity and consistency, you should use AuditAction.Unarchived, which exists in the AuditAction enum. The change summary already correctly states "Unarchived".
await SafeLogAsync("board", board.Id, AuditAction.Unarchived, changes: changeSummary);| if (dto.Name != null) | ||
| parts.Add($"Name: '{oldName}' -> '{dto.Name}'"); | ||
| if (dto.Description != null) | ||
| parts.Add($"Description changed"); |
There was a problem hiding this comment.
The current implementation of BuildBoardChangeSummary adds a change to the summary if a property in the DTO is not null, without checking if the new value is different from the old one. This can lead to noisy audit logs, for example, Name: 'My Board' -> 'My Board'. You should check if the value has actually changed before adding it to the summary.
if (dto.Name != null && dto.Name != oldName)
parts.Add($"Name: '{oldName}' -> '{dto.Name}'");
if (dto.Description != null && dto.Description != oldDescription)
parts.Add($"Description changed");| if (dto.Title != null) | ||
| parts.Add($"Title: '{oldTitle}' -> '{dto.Title}'"); | ||
| if (dto.Description != null) | ||
| parts.Add("Description changed"); | ||
| if (dto.DueDate.HasValue) | ||
| parts.Add($"DueDate: '{oldDueDate?.ToString("O") ?? "none"}' -> '{dto.DueDate.Value:O}'"); |
There was a problem hiding this comment.
The current implementation adds changes to the summary if properties in the DTO are not null or have a value, without checking if the new value is different from the old one. This can create noisy audit logs (e.g., Title: 'A' -> 'A'). Please add checks to ensure values have actually changed.
if (dto.Title != null && dto.Title != oldTitle)
parts.Add($"Title: '{oldTitle}' -> '{dto.Title}'");
if (dto.Description != null && dto.Description != oldDescription)
parts.Add("Description changed");
if (dto.DueDate.HasValue && dto.DueDate != oldDueDate)
parts.Add($"DueDate: '{oldDueDate?.ToString("O") ?? "none"}' -> '{dto.DueDate.Value:O}'");| if (dto.Name != null) | ||
| parts.Add($"Name: '{oldName}' -> '{dto.Name}'"); | ||
| if (dto.WipLimit.HasValue) | ||
| parts.Add($"WipLimit: {oldWipLimit?.ToString() ?? "none"} -> {dto.WipLimit.Value}"); | ||
| if (dto.Position.HasValue) | ||
| parts.Add($"Position: {oldPosition} -> {dto.Position.Value}"); |
There was a problem hiding this comment.
The current implementation adds a change to the summary if a property in the DTO is not null or has a value, without checking if the new value is different from the old one. This can lead to noisy audit logs, for example, Name: 'My Column' -> 'My Column'. You should check if the value has actually changed before adding it to the summary.
if (dto.Name != null && dto.Name != oldName)
parts.Add($"Name: '{oldName}' -> '{dto.Name}'");
if (dto.WipLimit.HasValue && dto.WipLimit != oldWipLimit)
parts.Add($"WipLimit: {oldWipLimit?.ToString() ?? "none"} -> {dto.WipLimit.Value}");
if (dto.Position.HasValue && dto.Position != oldPosition)
parts.Add($"Position: {oldPosition} -> {dto.Position.Value}");| if (dto.Name != null) | ||
| parts.Add($"Name: '{oldName}' -> '{dto.Name}'"); | ||
| if (dto.ColorHex != null) | ||
| parts.Add($"Color: '{oldColorHex}' -> '{dto.ColorHex}'"); |
There was a problem hiding this comment.
The current implementation adds a change to the summary if a property in the DTO is not null, without checking if the new value is different from the old one. This can lead to noisy audit logs, for example, Name: 'My Label' -> 'My Label'. You should check if the value has actually changed before adding it to the summary.
if (dto.Name != null && dto.Name != oldName)
parts.Add($"Name: '{oldName}' -> '{dto.Name}'");
if (dto.ColorHex != null && dto.ColorHex != oldColorHex)
parts.Add($"Color: '{oldColorHex}' -> '{dto.ColorHex}'");- CardService: skip logging Title/Description/DueDate when value matches old - BoardService: skip logging Name/Description when value matches old - ColumnService: skip logging Name/WipLimit/Position when value matches old - LabelService: skip logging Name/ColorHex when value matches old
- UpdateCard_WithSameTitle_DoesNotLogTitleChange - UpdateCard_BlockWithoutReason_DoesNotLogBlocked - UpdateBoard_WithUnarchiveFlag_RecordsUnarchivedAction
Gemini code review findings - fixedFix 1 (HIGH): Card block logic corrected (
|
1 participant
Summary
Closes #583
Test plan
dotnet testpasses clean across all projects